RFC: Comprehensive testing of basic constraint functionality #354

odow · 2018-05-11T07:30:36Z

Background

At the moment it is pretty hard from the solver's point-of-view to check that they are implementing the full MOI API correctly (e.g., LQOI). This PR attempts to test this.

For every constraint type supported by the solver, it tests the basic usage of the MOI API. It does not test corner cases. That can come later. In most cases, checking that the solver implemented something correctly will involve forming the problem, modifying, solving, and then checking the solution. This PR just tries to ensure that the methods are implemented.

It has exposed a number of bugs an un-implemented stuff in LQOI (ref JuliaOpt/LinQuadOptInterface.jl#10).

See the tests in Gurobi.jl for an example of how it can be called.

What this does

N.B. some unchecked things still todo.

In particular, it adds tests for basic usage of:

for the function-set combinations

~~Considering doing something like~~

const BasicConstraintTests = Dict( (MOI.SingleVariable, MOI.ZeroOne) => test_singlevariable_in_zeroone ) function basic_constraint_tests(model::MOI.ModelLike, config::TestConfig; delete::Bool=true) for ( (F,S), func ) in BasicConstraintTests if MOI.supportsconstraint(model, F, S) @testset "$(string(func))" begin func(model, config; delete=delete) end end end end unittests["basic_constraint_tests"] = basic_constraint_tests

~~But, we probably need a way for solvers to specify if they don't support deleting or querying a subset of constraint types.~~ I did something like this.

codecov-io · 2018-05-11T07:36:03Z

Codecov Report

Merging #354 into master will decrease coverage by 0.02%.
The diff coverage is 98.38%.

@@            Coverage Diff             @@
##           master     #354      +/-   ##
==========================================
- Coverage   96.27%   96.25%   -0.03%     
==========================================
  Files          36       37       +1     
  Lines        4723     4801      +78     
==========================================
+ Hits         4547     4621      +74     
- Misses        176      180       +4

Impacted Files	Coverage Δ
src/Test/UnitTests/unit_tests.jl	`100% <ø> (ø)`	⬆️
src/sets.jl	`77.41% <100%> (-0.71%)`	⬇️
src/Test/UnitTests/basic_constraint_tests.jl	`98.33% <98.33%> (ø)`
src/indextypes.jl	`62.5% <0%> (ø)`	⬆️
src/attributes.jl	`94.82% <0%> (+1.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e85ea35...ae6abf2. Read the comment docs.

odow · 2018-05-14T21:01:00Z

src/Test/UnitTests/basic_constraint_tests.jl

+    (MOI.VectorOfVariables, MOI.LogDetConeTriangle)               => ( dummy_vectorofvariables, 6, MOI.LogDetConeTriangle(3) ),
+    (MOI.VectorOfVariables, MOI.LogDetConeSquare)                 => ( dummy_vectorofvariables, 9, MOI.LogDetConeSquare(3) ),
+    (MOI.VectorOfVariables, MOI.RootDetConeTriangle)              => ( dummy_vectorofvariables, 6, MOI.RootDetConeTriangle(3) ),
+    (MOI.VectorOfVariables, MOI.RootDetConeSquare)                => ( dummy_vectorofvariables, 9, MOI.RootDetConeSquare(3) ),


@blegat are these right? I'm not sure if I understood the docs correctly.

Almost, you should do +1 (6->7 and 9->10) for the Det-cones to account for the t

You could check that with the dimension function

Maybe this should be clarified in the docs? It currently says:
"The argument dimension is the side dimension of the matrix X, i.e., its number of rows or columns."
I took that to mean excluding the t.

Yes, the dimension field excludes the t but the dimension function returns the size that a vector set should have to belong to that set so it includes t and all element of the matrices hence it is 1 + d^2 for LogDetSquare where d is the field

Oh right. I got confused. The docs are clear now that I read them properly.

odow · 2018-05-15T03:40:44Z

This is now RFC. It doesn't attempt to test each method thoroughly. It just tries to check that solvers have implemented the full API for all the F-in-S combinations they claim to support.

This helps avoid situations where you think you've implemented delete! for all constraints, only to find that you missed ScalarAffine-in-Interval. This was helpful for LQOI.

There are a few unchecked items that can probably be addressed in a new PR.

blegat · 2018-05-15T09:18:35Z

src/Test/UnitTests/basic_constraint_tests.jl

+    if length(include) > 0 && length(exclude) > 0
+        error("Don't use `include` and `exclude` together.")
+    end
+    test_keys = length(include) > 0 ? include : Iterators.filter(x->!(x in exclude), keys(BasicConstraintTests))


Why don't you do test_keys = keys(BasicConstraintTests) in the case where include and exclude are both empty. Since you do if MOI.supportsconstraint later, this would test all supported constraints and remove the need for a ListOfSupportedConstraintTypes discussed in #357 (comment)

This does test keys(BasicConstraintTests) if include and exclude are both empty.

If we had ListOfSupportedConstraintTypes, we could throw a warning that we didn't test SingleVariable-in-LessThan{Int} for example.

This does test keys(BasicConstraintTests) if include and exclude are both empty.

Indeed ^^

If we had ListOfSupportedConstraintTypes, we could throw a warning that we didn't test SingleVariable-in-LessThan{Int} for example.

The warning would be called when include is non-empty and it does not contain all supported sets ? This can already be done now by iterating through the keys of BasicConstraintTests

blegat · 2018-05-15T09:24:31Z

src/Test/UnitTests/basic_constraint_tests.jl

+            @test length(c_indices) == 3  # check that we've added a constraint
+            @test MOI.candelete(model, c_indices[1])
+
+            @test length(c_indices) == 3  # check that we've added a constraint


This check is already done three lines before

blegat · 2018-05-15T09:25:09Z

src/Test/UnitTests/basic_constraint_tests.jl

+
+    @testset "isvalid" begin
+        c_indices = MOI.get(model, MOI.ListOfConstraintIndices{F,S}())
+        @test length(c_indices) == 3  # check that we've added a constraint


I don't understand the comment

blegat · 2018-05-15T09:26:37Z

src/Test/UnitTests/basic_constraint_tests.jl

+# x₁, x₂
+const dummy_vectorofvariables = (x::Vector{MOI.VariableIndex}) -> MOI.VectorOfVariables(x)
+# 1.0 * x
+const dummy_scalar_affine     = (x::Vector{MOI.VariableIndex}) -> MOI.ScalarAffineFunction(x, [1.0], 0.0)


[1.0] should be replaced by ones(Float64, length(x))

mlubin · 2018-05-15T12:06:59Z

The naming of test_basic_constraint_functionality and basic_constraint_tests is a bit confusing. Are they both public functions that users should care about? They should have parallel names in this case. Is test_basic_constraint_functionality a helper for basic_constraint_tests? do_x is a confusing name for a helper for x_do.

odow · 2018-05-16T20:27:17Z

Merged this as is. We can address the modification tests after #360 is addressed.

Comprehensive testing of basic constraint functionality

d2299e4

odow changed the title ~~Comprehensive testing of basic constraint functionality~~ WIP: Comprehensive testing of basic constraint functionality May 11, 2018

odow added 3 commits May 12, 2018 11:50

More unit tests

1ae99c1

Add VoV and SOS tests

ea2d554

Add supportsconstrainttest

fc24d71

odow mentioned this pull request May 12, 2018

WIP: fix issues identified by MOI #354 JuliaOpt/LinQuadOptInterface.jl#10

Merged

odow added 5 commits May 12, 2018 14:26

VoV in vector sets

edd9c84

VAF in sets

46a0a0f

Move a few tests around

7ec5ff2

Rejig basic constraint tests

2993c52

VectorQuadratic

cd45109

This was referenced May 13, 2018

DNMY: MOI jump-dev/Gurobi.jl#125

Merged

DNMY: MOI jump-dev/GLPK.jl#53

Merged

rename ListOfConstraints? #214

Closed

odow added 2 commits May 14, 2018 11:17

Test some conic sets

6a2523a

Add conic constraints

7114fa4

odow commented May 14, 2018

View reviewed changes

odow mentioned this pull request May 14, 2018

Proposal: change API for modifications #360

Closed

odow changed the title ~~WIP: Comprehensive testing of basic constraint functionality~~ RFC: Comprehensive testing of basic constraint functionality May 15, 2018

blegat reviewed May 15, 2018

View reviewed changes

Correct dimension of some conic sets

ca41974

blegat reviewed May 15, 2018

View reviewed changes

odow added 2 commits May 15, 2018 21:26

Update comments

a7b74f5

More fixes for generality

88329b2

blegat approved these changes May 15, 2018

View reviewed changes

rename methods and reorg file

ae6abf2

mlubin approved these changes May 16, 2018

View reviewed changes

odow merged commit ed7331b into master May 16, 2018

odow deleted the odow/moreunit branch May 16, 2018 20:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RFC: Comprehensive testing of basic constraint functionality #354

RFC: Comprehensive testing of basic constraint functionality #354

odow commented May 11, 2018 •

edited

Loading

codecov-io commented May 11, 2018 •

edited

Loading

odow May 14, 2018

blegat May 15, 2018

blegat May 15, 2018

odow May 15, 2018

blegat May 15, 2018

odow May 15, 2018

odow commented May 15, 2018

blegat May 15, 2018

odow May 15, 2018

blegat May 15, 2018

blegat May 15, 2018

blegat May 15, 2018

blegat May 15, 2018

mlubin commented May 15, 2018

odow commented May 16, 2018

RFC: Comprehensive testing of basic constraint functionality #354

RFC: Comprehensive testing of basic constraint functionality #354

Conversation

odow commented May 11, 2018 • edited Loading

Background

What this does

codecov-io commented May 11, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

odow commented May 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlubin commented May 15, 2018

odow commented May 16, 2018

odow commented May 11, 2018 •

edited

Loading

codecov-io commented May 11, 2018 •

edited

Loading